Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/android/view builder and speed json #360

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Archdoog
Copy link
Collaborator

  • Converts Demo's ViewModel to extend DefaultNavigationViewModel
  • Fixes "speed" vs "value" key in SpeedUnit
  • Internalizes view builder params so that you can't accidentally try to customize them.
  • Adds a public get for custom view (used in maplibreui).

@@ -18,7 +18,7 @@ class SpeedSerializationAdapter : JsonAdapter<Speed>() {
is Speed.NoLimit -> writer.name("none").value(true)
is Speed.Unknown -> writer.name("unknown").value(true)
is Speed.Value ->
writer.name("value").value(speed.value).name("unit").value(speed.unit.text)
writer.name("speed").value(speed.value).name("unit").value(speed.unit.text)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmedre I believe the actual key should be "speed" when there's a value. E.g.

{
  "speed": 56,
  "unit": "km/h"
},

Let me know if there are additional cases. Definitely a bit of an obnoxious json object 😄.

Also, do we want to try catch the annotations parsing? On iOS we just convert a json parsing error to nil/null for annotations and an onError callback so that developers can log the failure. We figured better to let navigation succeed even in the case of a degraded/malformed annotations. See

do {
return try decoder.decode(Annotation.self, from: data)
} catch {
onError(error)
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are right - my mistake! agree re: try/catching the parsing with an error callback 👍

@@ -19,14 +19,16 @@ import com.stadiamaps.ferrostar.composeui.views.components.TripProgressView
import com.stadiamaps.ferrostar.core.NavigationUiState

data class NavigationViewComponentBuilder(
val instructionsView: @Composable (modifier: Modifier, uiState: NavigationUiState) -> Unit,
val progressView:
internal val instructionsView:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This avoids users accidentally trying to do:

NavigationViewComponentBuilder.Default()
    .instructionsView { }

Which is very easy to do and highly problematic.

@@ -24,7 +25,8 @@ import uniffi.ferrostar.UserLocation
class DemoNavigationViewModel(
// This is a simple example, but these would typically be dependency injected
private val ferrostarCore: FerrostarCore = AppModule.ferrostarCore,
) : ViewModel(), LocationUpdateListener, NavigationViewModel {
annotationPublisher: AnnotationPublisher<*> = valhallaExtendedOSRMAnnotationPublisher()
) : DefaultNavigationViewModel(ferrostarCore, annotationPublisher), LocationUpdateListener {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianthetechie now that this view model uses the DefaultNavigationViewModel. We should be able to refactor it to use the existing uiState from ferrostar and implement its own additional state for things like route loading, etc. Just to highlight how easy it is to customize without interrupting the default navigation behavior.

Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I'll wait for @ahmedre to comment since he may have an opinion based on what they're building 👍

@ahmedre
Copy link
Contributor

ahmedre commented Nov 15, 2024

On the DemoNavigationViewModel usage of DefaultNavigationViewModel - I think it is a step forward, but I think it is not as flexible as we have with just using FerrostarCore directly, because in this case, for example, both need to emit a NavigationUiState object.

We can swap NavigationUiState for a sealed class to solve this problem specifically for the demo use case of additional states like "not navigating" (which is what we also did in our implementation, but using FerrostarCore directly), but if we want to add lane support or speed limit support later, we'll need to either add these to NavigationUiState for everyone, or have our own ui state, or have two flows (in which case are we sure they're in sync, or would someone just need to combine them together to get a combination).

If we want to go this route, should we consider making NavigationUiState an interface with some required fields and let the return type be any instance? The downside of this approach though is that people would need an instance of check, and/or we'd need to do something with generics to avoid that. Alternatively, should we consider a single ViewModel for the demo app demoing the types of things you can do using FerrostarCore - it'd be opinionated and you could use it or use the formula from the ViewModel for building your own?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants